refactor(BA-5861): Share a single DockerStatsStreamer across CPU/Memory plugins#11234
Open
rapsealk wants to merge 5 commits into
Open
refactor(BA-5861): Share a single DockerStatsStreamer across CPU/Memory plugins#11234rapsealk wants to merge 5 commits into
rapsealk wants to merge 5 commits into
Conversation
… plugins Consolidate stream ownership onto DockerAgent so each container opens one persistent Docker stats stream instead of two (one per intrinsic plugin). - DockerAgent creates and owns a single DockerStatsStreamer in __ainit__, closes it in shutdown(). - Agent dispatches start/stop directly from container lifecycle events. - CPUPlugin / MemoryPlugin receive the shared streamer via attach_stats_streamer; per-plugin instantiation is removed. - Drop AbstractComputePlugin.notify_container_started/destroyed and the agent-side dispatcher - ownership now lives on the agent. Closes #11232 Refs #11216 Refs #11224 Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
github-actions
Bot
added
size:L
The consolidated DockerStatsStreamer is created in DockerAgent.__ainit__, but was placed after `await super().__ainit__()`. AbstractAgent's init calls scan_running_kernels() and starts the lifecycle handler, which on warm restart can fire container-start events before the streamer exists, raising AttributeError. - Move streamer creation + plugin attach loop before super().__ainit__(). - Drop the dead `is not None` guard in shutdown() (annotation is non-Optional; assignment happens synchronously before any await). - Switch the attach loop to hasattr(..., "attach_stats_streamer") so plugin subclasses are handled. - Add a test that asserts the streamer is attached before scan_running_kernels runs. Refs #11232 Refs #11234 Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
4 tasks
jopemachine
reviewed
Apr 24, 2026
| self._stats_streamer = DockerStatsStreamer(self.docker) | ||
| for computer_ctx in self.computers.values(): | ||
| instance = computer_ctx.instance | ||
| if hasattr(instance, "attach_stats_streamer"): |
Member
There was a problem hiding this comment.
We prefer to avoid dynamic access patterns such as hasattr whenever possible.
Member
Author
There was a problem hiding this comment.
Good point — removed the hasattr probe in 7cf076a. Moved attach_stats_streamer onto AbstractComputePlugin with a no-op default so non-Docker plugins (K8s, Dummy, third-party accelerators) inherit it safely, and DockerAgent.__ainit__ now calls it unconditionally on every compute plugin.
…er on base Move `attach_stats_streamer` to `AbstractComputePlugin` as a no-op default so `DockerAgent` can call it unconditionally on every compute plugin instead of probing with `hasattr`. CPU/Memory plugins keep their overrides; non-Docker plugins (K8s, Dummy, third-party accelerators) inherit the no-op safely. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The ordering test stubbed a non-intrinsic plugin with ``object()`` to exercise the removed ``hasattr`` skip branch; the attach loop now calls ``attach_stats_streamer`` unconditionally on every compute plugin, so the stub raises ``AttributeError``. Drop the unrelated-plugin case and the stale docstring reference to the hasattr branch — the no-op default on ``AbstractComputePlugin`` makes the skip structurally safe. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
rapsealk
changed the title
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #11232 (BA-5861)
Refs #11216
Summary
DockerStatsStreamerownership ontoDockerAgent— one streamer per agent instead of one per intrinsic plugin.start/stopdirectly via newAbstractAgent._on_container_started/_on_container_destroyedhooks; the old plugin-sidenotify_container_started/notify_container_destroyedinterface is removed.attach_stats_streamer()setter called beforeawait super().__ainit__()— the streamer is live beforescan_running_kernels()can inject START events into the lifecycle queue on warm restart.Why
At ~50+ containers, the per-plugin-streamer layout hit aiohttp's default
limit_per_host=30connector limit and stacked backoff retries. See #11232 for the full rationale.Stacked on
#11224 (
perf/11219-stream-container-stats). Rebase to main once #11224 merges.Test plan
pants test tests/unit/agent:: tests/component/agent/docker::passes — includes a regression test asserting_stats_streameris attached beforescan_running_kernelsruns (warm-restart ordering guard).AttributeError.ss -tnp | grep dockerd | wc -l.docker restartmid-session, verify streams re-establish and stats resume.